Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redis cache: make blocking executions unordered #42540

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Aug 14, 2024

When the Redis cache is invoked from an ordered Vert.x blocking execution, which happens for example with SmallRye GraphQL or with chained caching (one blocking @CacheResult method invoking other blocking @CacheResult method), the Redis cache ends up hanging. This is because the next execution cannot start until the previous execution finishes, but the previous execution waits for the next execution.

This commit fixes that by making the Redis cache blocking executions unordered.

Fixes #39558
Fixes #42253

When the Redis cache is invoked from an ordered Vert.x blocking execution,
which happens for example with SmallRye GraphQL or with chained caching
(one blocking `@CacheResult` method invoking other blocking `@CacheResult`
method), the Redis cache ends up hanging. This is because the next execution
cannot start until the previous execution finishes, but the previous execution
waits for the next execution.

This commit fixes that by making the Redis cache blocking executions unordered.
Copy link

quarkus-bot bot commented Aug 14, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 1fd2a9a.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@gsmet
Copy link
Member

gsmet commented Aug 14, 2024

I don’t know this code at all but it feels dangerous to me to unorder cache calls execution, especially the writes.

@Ladicek
Copy link
Contributor Author

Ladicek commented Aug 15, 2024

@gsmet Right, so unordering here doesn't mean changing the order of operations expressed as Uni pipelines in the code. It refers to Vert.x executeBlocking()'s ordered parameter, which if set to true means that the [blocking] operations execute on the same thread; in other words, they are ordered. I believe that is wrong here, because if the caller of the @CacheResult method is already an ordered blocking execution, then we're effectively in a deadlock situation. The caller waits for the @CacheResult method to return, the @CacheResult method waits for the blocking execution to proceed, and it cannot, because it never gets its chance. By unordering the @CacheResult calls, we make sure they can [and will] be executed on different threads, preventing the deadlock. Hope that helps.

@gsmet
Copy link
Member

gsmet commented Aug 15, 2024

Sounds a bit less scary but maybe let’s wait for @cescoffier ? :)

@Ladicek
Copy link
Contributor Author

Ladicek commented Aug 16, 2024

I'm not merging this, but note that @cescoffier has already approved :-)

@gsmet
Copy link
Member

gsmet commented Aug 16, 2024

I have a filter hiding approvals made by people on PTO :).

@gsmet gsmet merged commit 9196d18 into quarkusio:main Aug 16, 2024
20 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.16 - main milestone Aug 16, 2024
@gsmet
Copy link
Member

gsmet commented Aug 16, 2024

Now the big question: should it end up in 3.14?

@Ladicek Ladicek deleted the redis-cache-unordered branch August 16, 2024 10:42
@Ladicek
Copy link
Contributor Author

Ladicek commented Aug 16, 2024

One of the fixed bugs is on the 3.15 LTS board, but I'm fine with leaving this to 3.16 only, personally.

@gsmet
Copy link
Member

gsmet commented Aug 16, 2024

@cescoffier I will let you judge of that and add the label for backport to 3.14 if you think that's how it should be done.

@cescoffier
Copy link
Member

Added the label.

Here, things must be unordered, but don't worry, it's ordered elsewhere (there is a Redis "transaction").

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants